Rework CoreUtility.deleteContent() for Java 25 compatibility and symlink safety #2159
Conversation
|
Note I tested this on Linux and Windows. No testing was done on MacOS |
merks
left a comment
merks
left a comment
There was a problem hiding this comment.
I see there exists org.eclipse.osgi.storage.StorageUtil.rm(File, Debug) which also would suffer from the read-only problem, which is a problem I encounter with my SBOM work where I also have similar a utility:
Also this utility:
This does this the old way:
org.eclipse.help.internal.search.SearchIndex.deleteDir(File)
The above is just an observation of how many places solve this problem with "duplicate" code...
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/DeleteContentWalker.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/DeleteContentWalker.java
Outdated
Show resolved
Hide resolved
| // only use the root content for progress. anything else would | ||
| // be overkill. | ||
| long count = stream.count(); | ||
| submonitor = SubMonitor.convert(monitor, (int) count); |
There was a problem hiding this comment.
The logic here is intricately tied to the if (Objects.equals(path.getParent(), root)) { guards in the walker. One might instead have specialized logic during preVisitDirectory for create a monitor only for the root.
In the end, this monitor will do a less good job of showing progress in a large branch. That makes me wonder if some kind of "stack" of monitors managed by preVisitDirectory/postVisitDirectory might not provide better progress? (Of course I'm not sure when/where such progress is displayed to the user, if anywhere.)
There was a problem hiding this comment.
In the end, this monitor will do a less good job of showing progress in a large branch. That makes me wonder if some kind of "stack" of monitors managed by preVisitDirectory/postVisitDirectory might not provide better progress? (Of course I'm not sure when/where such progress is displayed to the user, if anywhere.)
I intentionally did id like this. I consider the progress monitoring here as very low priority. In my experience, the user does not care if 56% or 57% of the job is done. The progress bar is mostly used as an activity indicator that signals to the user that stuff is happening.
Using a stack of monitors would be more correct but also wrong since some directories are empty, other might by very big. In my opinion, it is a waist of time to optimize the progress monitoring.
There was a problem hiding this comment.
Yes, I don't disagree about the value of the monitor. It's just an observation.
That being said, I had to go hunting in the code to understand how this monitor count related to how it's actually used in the code.
DieterMai
force-pushed
the
master
branch
2 times, most recently
from
d73e3ee to
3b821e3
Compare
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/DeleteContentWalker.java
Outdated
Show resolved
Hide resolved
The previous implementation had the following flaws: - On Linux, soft links to directories were followed, causing files outside the workspace to be deleted. - On Windows, the read-only flag was not removed explicitly. Before Java 25, this was done implicitly by the File.delete() method. Since Java 25, the flag must be removed before calling File.delete() or Files.delete(). See JDK-8355954. This PR changes the implementation to: - Use the NIO file walker to iterate the root directory. - Avoid following symlinks. - Remove the Windows read-only flag explicitly. Signed-off-by: Dieter Mai <[email protected]>
2 participants
The previous implementation had the following flaws:
File.delete()method. Since Java 25, the flag must be removed before callingFile.delete()orFiles.delete(). See JDK-8355954.This PR changes the implementation to:
Resolves #1958